[Data masking] Ensure @unmask can be used with the fragment registry
#12029
Conversation
|
| @@ -170,11 +148,18 @@ export abstract class ApolloCache<TSerialized> implements DataProxy { | |||
| // If not implemented by a cache subclass, data masking will effectively be | |||
| // disabled since we will not be able to accurately determine if a given type | |||
| // condition for a union or interface matches a particular type. | |||
| protected fragmentMatches?( | |||
| public fragmentMatches?( | |||
There was a problem hiding this comment.
I'm not super fond of this name and perhaps this PR is a good time to change it (this was introduced with data masking by the way). This function is essentially responsible for the possibleTypes-like API which map interface/union types to concrete types. This ensures we can traverse the right selection set when given a type in a response that is part of an interface.
InMemoryCache has the possibleTypes API, but not all caches are guaranteed to have this. Any ideas on a more descriptive name here?
There was a problem hiding this comment.
Some possibilities:
fragmentMatchesTypenamefragmentMatchesPolymorphicTypefragmentConditionIncludesTypename
I'm not a huge fan of these either, but I guess what I don't like about the original name is that it feels like it needs something after the "matches" part to understand what its matching against. I just can't seem to put my finger on the right word to use here.
There was a problem hiding this comment.
What do you think about splitting up part of the policies.fragmentMatches(fragment, typename, result, variables) api into a policies.satisfiesSupertype(typename, supertype)?
result and variables are both optional, and if they are not given, the only fragment property that is acessed is fragment.typeCondition anyways.
This would result in a satisfiesSupertype(typename, supertype) api here without any relationship to fragment, with a call like context.cache.satisfiesSupertype!(data.__typename, selection.typeCondition) in masking.ts.
Alternative name: satisfiesTypeCondition or implementsSupertype.
That would also make is a lot less intimidating for other caches to implement this method - fragmentMatches implies a lot more functionality (and our implementation does a lot more unrelated stuff).
There was a problem hiding this comment.
I like these suggestions way more than anything I had and dig the idea of having a separate policies function for this. For that, I'll split that out to a separate PR so that I can experiment with it and don't bury it in this one.
| // Function used to lookup a fragment when a fragment definition is not part | ||
| // of the GraphQL document. This is useful for caches, such as InMemoryCache, | ||
| // that register fragments ahead of time so they can be referenced by name. | ||
| public lookupFragment(fragmentName: string): FragmentDefinitionNode | null { |
There was a problem hiding this comment.
Do we like this more terse, or do we prefer something a bit more verbose like lookupFragmentByName?
…g to lookup a fragment by name.
jerelmiller
force-pushed
the
jerel/fix-fragment-matching-fragment-registry
branch
from
ee1488e to
2b6beb2
Compare
| ): TData { | ||
| if (!cache.fragmentMatches) { |
There was a problem hiding this comment.
A lot of these changes were just moved from original cache implementation
| // Function used to lookup a fragment when a fragment definition is not part | ||
| // of the GraphQL document. This is useful for caches, such as InMemoryCache, | ||
| // that register fragments ahead of time so they can be referenced by name. | ||
| public lookupFragment(fragmentName: string): FragmentDefinitionNode | null { |
Co-authored-by: Lenz Weber-Tronic <[email protected]>
jerelmiller
force-pushed
the
jerel/fix-fragment-matching-fragment-registry
branch
from
96d1a49 to
5dbab89
Compare
#12029) Co-authored-by: Lenz Weber-Tronic <[email protected]>
#12029) Co-authored-by: Lenz Weber-Tronic <[email protected]>
#12029) Co-authored-by: Lenz Weber-Tronic <[email protected]>
#12029) Co-authored-by: Lenz Weber-Tronic <[email protected]>
#12029) Co-authored-by: Lenz Weber-Tronic <[email protected]>
Fixes #12028
Ensures that fragments registered with the fragment registry can be looked up by the data masking algorithm.
This change adds a new
lookupFragmentAPI in the base cache class that allows custom caches to provide lookups for fragments by name. For example,InMemoryCachehas the fragment registry, so it overrides the base implementation and uses the fragment registry to lookup a fragment by name.As a part of this, I've moved the implementation of
maskFragmentandmaskOperationover toQueryManagerwhere I believe it fits better. This makes it a much more private API and makes it much more obvious that custom cache implementations don't need to interact with these APIs directly since we want the core Apollo Client controlling the implementation.